Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update config file specification format #128

Merged
merged 10 commits into from
Jul 26, 2024
Merged

Update config file specification format #128

merged 10 commits into from
Jul 26, 2024

Conversation

dragonejt
Copy link
Contributor

@dragonejt dragonejt commented Mar 23, 2024

Description

Testing

  • python driver.py ran with no errors
  • python driver.py -t (all unit tests) succeeded

Considerations

  • I had used VSCode's autoformatter to format test_execution_engine.py when updating the fake data to verify the fake data was nested correctly. If we want to go back to the original format for some reason I can figure something out to revert the formatting, but as an action item I can also take a look at adding a linter and autoformatter if we want to ensure a consistent code style.
  • I had posted a comment in Update section and key names in config files for clarity and code usage #101 about it being difficult to find any references for the .ini file specification. Has it been considered to move to a more modern/commonly used python configuration file format such as TOML (which visually appears very similar to .ini)? Python has a built-in library for reading TOML files as well, and modern python libraries use a pyproject.toml file for configurations.
  • EDIT: I have not been able to make the OPTIONS section optional, will modify functionality in the next commit

@dragonejt
Copy link
Contributor Author

@the-other-james @asgibson just in case y'all haven't seen this PR, I updated the config file specification format to what was wanted in #128

@asgibson
Copy link
Contributor

asgibson commented Apr 1, 2024

@dragonejt at a glance your coding looks very good, but my formal review will take a little more time. After my review, it will need a secondary review before a merge. There are two reviews ahead in the pipeline #125 and #127 (#98 is on hold).

In your comment, you mention #128, but as this comment is in 128, I assume you mean the issue #101. In my cursory review you appear to be hitting all the marks on the requests in that issue. While awaiting the formal review, are you able to create a feature branch for #101 and rebase it to branch 126? We like to use branches for issue tracking and the rebase will make merging smoother after the other PRs are complete.

Thank you very much for your time and effort, it is appreciated!

@dragonejt
Copy link
Contributor Author

dragonejt commented Apr 1, 2024

Sorry, yes I meant that this PR addresses #101 . Unfortunately, as I am not a collaborator on this repository I cannot create new branches on the repo or push to any branch on the repo. Instead, I forked the repository and submitted the PR from the fork to the original, as that was the guidance from reading this. According to that doc, there is the option to be added as a collaborator to the repository on a case-by-case basis if that is something that we want to pursue. Otherwise, the best I can do is currently what is being done, which is forking the repo and submitting the PR from my fork onto the original repository.

@asgibson
Copy link
Contributor

asgibson commented Apr 1, 2024

No worries, I thought that may be the case. Thank you for looking into it.

Copy link
Contributor

@the-other-james the-other-james left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! And thank you for your patience with us!

Before I approve, please update the branch you are merging in to have the latest from our main. I think there are likely conflicts with the redis .ini file since Alan had made updates.

Also, please address my annoying nitpicks about formatting :)

Thanks!

.gitignore Outdated Show resolved Hide resolved
onair/config/default_config.ini Show resolved Hide resolved
onair/src/run_scripts/execution_engine.py Show resolved Hide resolved
Copy link
Contributor

@asgibson asgibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! All 8 of the requested changes in #101 are addressed.
Some minor changes to make, but overall looks great.

onair/config/default_config.ini Show resolved Hide resolved
onair/config/reporter_config.ini Outdated Show resolved Hide resolved
onair/config/redis_example.ini Show resolved Hide resolved
onair/config/default_config.ini Show resolved Hide resolved
onair/config/default_config.ini Outdated Show resolved Hide resolved
onair/src/run_scripts/execution_engine.py Outdated Show resolved Hide resolved
test/onair/src/run_scripts/test_execution_engine.py Outdated Show resolved Hide resolved
test/onair/src/run_scripts/test_execution_engine.py Outdated Show resolved Hide resolved
test/onair/src/run_scripts/test_execution_engine.py Outdated Show resolved Hide resolved
test/onair/src/run_scripts/test_execution_engine.py Outdated Show resolved Hide resolved
@dragonejt
Copy link
Contributor Author

Sorry for the delay, I've been busy. I'll work on these changes over the next few weeks and submit a revision.

@dragonejt
Copy link
Contributor Author

Hey sorry I haven't gotten to revising this PR, I'm going to address the comments here over the next week or so. Are there any updates to how we use the .ini file that need to be reflected in this PR? Also, looking at the formatting changes requested, I was thinking about bringing in a Python code formatter so that I didn't need to manually format the codebase, such as black or Ruff. Is there any reason I should not use an autoformatter?

@dragonejt
Copy link
Contributor Author

dragonejt commented Jun 18, 2024

Rev 2

  • Remove Dev_enabled and Viz_enabled feature flags
  • Replace deprecated distutils.copy_tree with new shutil.copytree
  • Add all items in GitHub's default python .gitignore with a newline at the bottom. This covers all files previously ignored, and more files that we should probably ignore.
  • Updated a unit test failure by changing how a class instance was created, please let me know if we need to change it back
FAILED test/onair/src/run_scripts/test_execution_engine.py::test_ExecutionEngine_parse_configs_skips_OPTIONS_when_the_required_section_OPTIONS_is_not_in_config - AttributeError: 'ExecutionEngine' object has no attribute 'IO_Enabled'

I believe __new__ doesn't actually call __init__, so changing it to how normal class instances are created calls __init__.

Testing

All Unit Tests succeeded.

@the-other-james
Copy link
Contributor

Could you please remove the changes related to formatting? I don't want to mix formatting changes in with your other changes: it makes for a huge pull request (62 changed files) which obfuscates the config file update.

Reformatting of the entire code base should be done as its own pull request. Alan started looking into this a while ago: #98

@dragonejt
Copy link
Contributor Author

A no-formatting revision has been pushed.

@dragonejt
Copy link
Contributor Author

Just in case you two haven't seen this, @the-other-james @asgibson

Copy link
Contributor

@the-other-james the-other-james left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Tested default config and unit tests on my machine.

Copy link
Contributor

@the-other-james the-other-james left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bit more testing and noticed that the kalman and redis configs hadn't been fully updated. I'm not sure if I can directly push my fixes to your merge request, so here's the diff that got things working on my end:

diff --git a/onair/config/kalman_csv_output_example.ini b/onair/config/kalman_csv_output_example.ini
index 06dab42..a1c927d 100644
--- a/onair/config/kalman_csv_output_example.ini
+++ b/onair/config/kalman_csv_output_example.ini
@@ -1,14 +1,17 @@
-[DEFAULT]
-TelemetryDataFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
+[FILES]
+TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
-TelemetryMetadataFilePath = onair/data/telemetry_configs/
+MetaFilePath = onair/data/telemetry_configs/
 MetaFile = data_physics_generation_CONFIG.json
-ParserFileName = onair/data_handling/csv_parser.py

+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/csv_parser.py
+
+[PLUGINS]
 KnowledgeRepPluginDict = {'Kalman Filter': 'plugins/kalman'}
 LearnersPluginDict = {'csv output':'plugins/csv_output'}
 PlannersPluginDict = {}
 ComplexPluginDict = {}

-[RUN_FLAGS]
-IO_Flag = true
+[OPTIONS]
+IO_Enabled = true
diff --git a/onair/config/redis_example.ini b/onair/config/redis_example.ini
index b0a599b..3f70e42 100644
--- a/onair/config/redis_example.ini
+++ b/onair/config/redis_example.ini
@@ -3,7 +3,9 @@ TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
 MetaFilePath = onair/data/telemetry_configs/
 MetaFile = redis_example_CONFIG.json
-ParserFileName = onair/data_handling/redis_adapter.py
+
+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/redis_adapter.py

 [PLUGINS]
 KnowledgeRepPluginDict = {'knowledge':'plugins/generic/__init__.py'}

@dragonejt
Copy link
Contributor Author

I did a bit more testing and noticed that the kalman and redis configs hadn't been fully updated. I'm not sure if I can directly push my fixes to your merge request, so here's the diff that got things working on my end:

diff --git a/onair/config/kalman_csv_output_example.ini b/onair/config/kalman_csv_output_example.ini
index 06dab42..a1c927d 100644
--- a/onair/config/kalman_csv_output_example.ini
+++ b/onair/config/kalman_csv_output_example.ini
@@ -1,14 +1,17 @@
-[DEFAULT]
-TelemetryDataFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
+[FILES]
+TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
-TelemetryMetadataFilePath = onair/data/telemetry_configs/
+MetaFilePath = onair/data/telemetry_configs/
 MetaFile = data_physics_generation_CONFIG.json
-ParserFileName = onair/data_handling/csv_parser.py

+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/csv_parser.py
+
+[PLUGINS]
 KnowledgeRepPluginDict = {'Kalman Filter': 'plugins/kalman'}
 LearnersPluginDict = {'csv output':'plugins/csv_output'}
 PlannersPluginDict = {}
 ComplexPluginDict = {}

-[RUN_FLAGS]
-IO_Flag = true
+[OPTIONS]
+IO_Enabled = true
diff --git a/onair/config/redis_example.ini b/onair/config/redis_example.ini
index b0a599b..3f70e42 100644
--- a/onair/config/redis_example.ini
+++ b/onair/config/redis_example.ini
@@ -3,7 +3,9 @@ TelemetryFilePath = onair/data/raw_telemetry_data/data_physics_generation/Errors
 TelemetryFile = 700_crash_to_earth_1.csv
 MetaFilePath = onair/data/telemetry_configs/
 MetaFile = redis_example_CONFIG.json
-ParserFileName = onair/data_handling/redis_adapter.py
+
+[DATA_HANDLING]
+DataSourceFile = onair/data_handling/redis_adapter.py

 [PLUGINS]
 KnowledgeRepPluginDict = {'knowledge':'plugins/generic/__init__.py'}

Ah, I may have not fully changed the other configs, sorry about that. I'll get that fixed

@dragonejt
Copy link
Contributor Author

@the-other-james It looks like what happened was that I did indeed miss the error in redis_example.ini, but the commit that added onair/config/kalman_csv_output_example.ini took place after I created my fork from the main repo, so my fork didn't have that file. The new commit I just pushed has the updates, and should work now.

Copy link
Contributor

@asgibson asgibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the ExecutionEngine instantiations in the unit tests. By only allocating space for the instances it makes the tests more maintainable because changes in the __init__ function will not affect the tests (as it will not run). This also requires the tests to explicitly set any attributes within the test itself because initialization was not performed on the instance, which provides more information about how the code under test (cut) operates.

test/onair/src/run_scripts/test_execution_engine.py Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@dragonejt
Copy link
Contributor Author

@asgibson the ExecutionEngine issue has been fixed, and I left a comment about where I got my Python .gitignore from. Please take a look when you have time.

Copy link
Contributor

@the-other-james the-other-james left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thank you for your patience!

@dragonejt
Copy link
Contributor Author

Hello, I don't have permissions to merge into the main branch, so one of you will need to merge whenever we're ready to merge.

@asgibson asgibson merged commit c59d04d into nasa:main Jul 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants